Fix priority between network and DHCP server #2269

dharaneeshvrd · 2025-03-24T13:10:59Z

What this PR does / why we need it:
Fix priority between network and DHCP server
Do not allow to set both network and DHCP server details which will create conflict while creating new DHCP server on whether to use network name or DHCP server name

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2159

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Fix priority between network and DHCP server

netlify · 2025-03-24T13:11:20Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`1af0a19`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/681041030d6ef40008002190
😎 Deploy Preview	https://deploy-preview-2269.cluster-api-ibmcloud.sigs.k8s.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dharaneeshvrd · 2025-03-24T13:11:22Z

/cc @Karthik-K-N

Karthik-K-N

https://github.com/Karthik-K-N/cluster-api-provider-ibmcloud/blob/c4171f21acc320fdfe17b812c98be908653134f7/cloud/scope/powervs_cluster.go#L870, This comment needs an update as now we are not allowing to set both network and DHCP details in spec.

Karthik-K-N · 2025-03-25T11:28:32Z

api/v1beta2/ibmpowervscluster_webhook_test.go

+					},
+					DHCPServer: &DHCPServer{
+						Name: ptr.To("capi-dhcp"),
+					},


May lets also add test with network name - dhcp id , network id -dhcp id combination?

dharaneeshvrd · 2025-03-26T15:13:49Z

Addressed the comments, ptal @Karthik-K-N

Karthik-K-N

Do you think is there a way to handle/inform about this scenario, Correct me if my understanding is wrong.

What if user sets network.Name="abc" the network "abc" does not exist in cloud. I think our controller will create a DHCP network DHCPSERVERabc_Private. Now user has set "abc" as network in spec but actually its DHCPSERVERabc_Private

dharaneeshvrd · 2025-03-28T08:32:06Z

@Karthik-K-N
Yes that's correct. I checked is there any update in DHCP create payload to accept the network name, seems it is not there.
So added this info into the API description as well as ReconcileNetwork()'s description. Please take a look

Karthik-K-N · 2025-03-28T10:26:30Z

True, Thats the only thing we can do, along with that should we update spec.Network.Name with the newly created DHCP network name? will that be allowed?

dharaneeshvrd · 2025-04-07T04:41:23Z

should we update spec.Network.Name with the newly created DHCP network name? will that be allowed?

Can you please elaborate this? Do you mean to update the spec once we created the DHCP network? Just wondering since we won't follow that generally right!

Karthik-K-N · 2025-04-07T09:48:50Z

should we update spec.Network.Name with the newly created DHCP network name? will that be allowed?

Can you please elaborate this? Do you mean to update the spec once we created the DHCP network? Just wondering since we won't follow that generally right!

True we wont follow it, Just thinking.

Karthik-K-N · 2025-04-07T13:21:21Z

Or else how about strict validation like ID, when network.Name is set it should exist or else return error?

Most of the time we ommit network field and controller will autocreate the resource so it wont be a breaking change.

dharaneeshvrd · 2025-04-07T13:36:08Z

Yeah that might be an option if we consider that field only for non DHCP networks. Anyway we are validating with that intention only. But only concern is that it would deviate from the other places on how we treat the name field of a resource.

Karthik-K-N · 2025-04-07T13:42:49Z

Yeah that might be an option if we consider that field only for non DHCP networks. Anyway we are validating with that intention only. But only concern is that it would deviate from the other places on how we treat the name field of a resource.

True that, Though the use case is very minimal, I am not so sure about showing wrong info about network name in spec.

May be lets discuss this in the community meeting and we can either go with that or keep as it is and add note like now.

Do not allow to set both network and DHCP server details which will create conflict while creating new DHCP server on whether to use network name or DHCP server name

Karthik-K-N · 2025-04-29T05:59:11Z

Discussed about it in the CAPIBM community meeting, We will keep the flow as is and not add a validation for network.Name like network.ID.

Need to setup another meeting to decide on how to allow users to create DHCP or static network.

Karthik-K-N

/lgtm

Prajyot-Parab

/lgtm

k8s-ci-robot · 2025-05-01T03:52:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharaneeshvrd, Prajyot-Parab

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Prajyot-Parab]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Prajyot-Parab · 2025-05-01T03:52:21Z

/hold

Prajyot-Parab · 2025-05-01T03:52:36Z

/test all

Prajyot-Parab · 2025-05-01T08:03:07Z

/unhold

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Mar 24, 2025

k8s-ci-robot requested review from Amulyam24 and mkumatag March 24, 2025 13:11

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 24, 2025

k8s-ci-robot requested a review from Karthik-K-N March 24, 2025 13:11

Karthik-K-N reviewed Mar 25, 2025

View reviewed changes

dharaneeshvrd force-pushed the fix-nw-priority branch from f76ccab to 09862db Compare March 26, 2025 15:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2025

Karthik-K-N reviewed Mar 28, 2025

View reviewed changes

dharaneeshvrd force-pushed the fix-nw-priority branch from 09862db to 7cfc5ac Compare March 28, 2025 08:29

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2025

dharaneeshvrd force-pushed the fix-nw-priority branch from 7cfc5ac to f46c35c Compare April 7, 2025 13:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2025

dharaneeshvrd force-pushed the fix-nw-priority branch from f46c35c to 6e0923b Compare April 29, 2025 02:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2025

Fix priority between network and DHCP server

1af0a19

Do not allow to set both network and DHCP server details which will create conflict while creating new DHCP server on whether to use network name or DHCP server name

dharaneeshvrd force-pushed the fix-nw-priority branch from 6e0923b to 1af0a19 Compare April 29, 2025 03:01

Karthik-K-N reviewed Apr 29, 2025

View reviewed changes

k8s-ci-robot assigned Karthik-K-N Apr 29, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2025

Prajyot-Parab approved these changes May 1, 2025

View reviewed changes

k8s-ci-robot assigned Prajyot-Parab May 1, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2025

k8s-ci-robot merged commit 9414bc3 into kubernetes-sigs:main May 1, 2025
14 checks passed

Fix priority between network and DHCP server #2269

Fix priority between network and DHCP server #2269

Uh oh!

Conversation

dharaneeshvrd commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Uh oh!

dharaneeshvrd commented Mar 24, 2025

Uh oh!

Karthik-K-N left a comment

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

dharaneeshvrd commented Mar 26, 2025

Uh oh!

Karthik-K-N left a comment

Choose a reason for hiding this comment

Uh oh!

dharaneeshvrd commented Mar 28, 2025

Uh oh!

Karthik-K-N commented Mar 28, 2025

Uh oh!

dharaneeshvrd commented Apr 7, 2025

Uh oh!

Karthik-K-N commented Apr 7, 2025

Uh oh!

Karthik-K-N commented Apr 7, 2025

Uh oh!

dharaneeshvrd commented Apr 7, 2025

Uh oh!

Karthik-K-N commented Apr 7, 2025

Uh oh!

Karthik-K-N commented Apr 29, 2025

Uh oh!

Karthik-K-N left a comment

Choose a reason for hiding this comment

Uh oh!

Prajyot-Parab left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 1, 2025

Uh oh!

Prajyot-Parab commented May 1, 2025

Uh oh!

Prajyot-Parab commented May 1, 2025

Uh oh!

Prajyot-Parab commented May 1, 2025

Uh oh!

Uh oh!

Uh oh!

dharaneeshvrd commented Mar 24, 2025 •

edited

Loading

netlify bot commented Mar 24, 2025 •

edited

Loading